Alternative style for CheckboxRadioSwitch (radio type) #2757

julien-nc · 2022-06-10T16:07:57Z

This was inspired by nextcloud/server#26691

A new boolean prop buttonVariant toggles between the classic style and this one. It can be used with all types.

If the CheckboxRadioSwitch elements are displayed next to each other vertically, their borders are "merged".

radio3.mp4

…ype only Signed-off-by: Julien Veyssier <eneiluj@posteo.net>

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>

jancborchardt

Nice! :) Some design feedback:

The last entry without icon should probably still be indented so the text is left-aligned with the others.
Currently each individual item has a border-radius on all corners – but actually it is a group which should only have border-radius on the 4 outside corners. So only on the top left and top right of the first entry and the bottom left and bottom right of the last entry.
Since we are increasing border-radius across the board, it’s probably best to go with var(--border-radius-large) for this

julien-nc · 2022-06-17T12:33:35Z

@jancborchardt Thanks for the feedback

The last entry without icon should probably still be indented so the text is left-aligned with the others.

The content is a slot. Its styling is done outside this component 😁.

Since we are increasing border-radius across the board, it’s probably best to go with var(--border-radius-large) for this

Yep.

Currently each individual item has a border-radius on all corners...

Completely agree... but: I did it like that initially and then I realized the radio elements are not necessarily next to each other so we need to handle both cases, IMO:

when they are next to each other, only round corners of first and last elements
when they don't "touch" each other, round each corners of each element

The main problem here is that it's impossible to select a predecessor in CSS (only a successor with +). Apparently it will come with CSS4 but whatever.

An alternative is to add props to set border-radius manually. This might make more sense because it's weird to try to make a component aware of what's around it in order to define its own style.

The video in the first comment has been updated.

skjnldsv · 2022-06-17T12:48:25Z

src/components/CheckboxRadioSwitch/CheckboxRadioSwitch.vue

+ /**
+ * Toggle the top left and right rounded corners for the alternative button style
+ */
+ buttonVariantFirst: {
+ type: Boolean,
+ default: false,
+ },
+
+ /**
+ * Toggle the bottom left and right rounded corners for the alternative button style
+ */
+ buttonVariantLast: {
+ type: Boolean,
+ default: false,
+ },


This should be automatically calculated.
Grouped-by-name radio can easily be targeted with first and last :)

Alright, now using :first-of-type and :last-of-type.

We still need to get the information about whether they are direct successors or not (grouped or not). Still using a prop for that.

I also added the ability to group vertically (video is up-to-date in the first comment).

We still need to get the information about whether they are direct successors or not (grouped or not). Still using a prop for that.

I don't think that's necessary (or even helpful). Grouped checkboxes need to be direct siblings with no checkboxes as siblings that are not grouped. So they need to be wrapped in a common container. Otherwise the markup will fail, without or with the grouped prop.

See this for example:

<template> <div> <div> First toggle with three options <CheckboxRadioSwitch :checked.sync="sharingPermission" value="r" name="sharing_permission_radio" type="radio" :button-variant="true" :button-variant-v-grouped="true">Default permission read</CheckboxRadioSwitch> <CheckboxRadioSwitch :checked.sync="sharingPermission" value="rw" name="sharing_permission_radio" type="radio" :button-variant="true" :button-variant-v-grouped="true">Default permission read+write</CheckboxRadioSwitch> <CheckboxRadioSwitch :checked.sync="sharingPermission" value="rw" name="sharing_permission_radio" type="radio" :button-variant="true" :button-variant-v-grouped="true">Default permission read+write</CheckboxRadioSwitch> </div> <div> Second toggle with two options <CheckboxRadioSwitch :checked.sync="sharingPermission" value="r" name="sharing_permission_radio" type="radio" :button-variant="true" :button-variant-v-grouped="true">Default permission read</CheckboxRadioSwitch> <CheckboxRadioSwitch :checked.sync="sharingPermission" value="rw" name="sharing_permission_radio" type="radio" :button-variant="true" :button-variant-v-grouped="true">Default permission read+write</CheckboxRadioSwitch> Other toggle not grouped <CheckboxRadioSwitch :checked.sync="sharingPermission" value="rw" name="sharing_permission_radio" type="radio" :button-variant="true">Default permission read+write</CheckboxRadioSwitch> <br> sharingPermission: {{ sharingPermission }} </div> </div> </template> <script> export default { data() { return { sharingPermission: 'r', } } } </script>

Which gives:

Notice that the corners of the second last checkbox are missing, since there is another one not belonging to the group afterwards. For the three grouped checkboxes wrapped in a container, it works fine.

So imho we only need a prop to indicate horizontal/vertical grouping (would be nice to figure that out by CSS as well, but I don't know how) and leave the rest to the proper markup (give a common container to the grouped boxes).

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>

src/components/CheckboxRadioSwitch/CheckboxRadioSwitch.vue

raimund-schluessler · 2022-06-20T10:22:07Z

src/components/CheckboxRadioSwitch/CheckboxRadioSwitch.vue

+ /**
+ * Toggle the top left and right rounded corners for the alternative button style
+ */
+ buttonVariantFirst: {
+ type: Boolean,
+ default: false,
+ },
+
+ /**
+ * Toggle the bottom left and right rounded corners for the alternative button style
+ */
+ buttonVariantLast: {
+ type: Boolean,
+ default: false,
+ },


We still need to get the information about whether they are direct successors or not (grouped or not). Still using a prop for that.

I don't think that's necessary (or even helpful). Grouped checkboxes need to be direct siblings with no checkboxes as siblings that are not grouped. So they need to be wrapped in a common container. Otherwise the markup will fail, without or with the grouped prop.

See this for example:

<template> <div> <div> First toggle with three options <CheckboxRadioSwitch :checked.sync="sharingPermission" value="r" name="sharing_permission_radio" type="radio" :button-variant="true" :button-variant-v-grouped="true">Default permission read</CheckboxRadioSwitch> <CheckboxRadioSwitch :checked.sync="sharingPermission" value="rw" name="sharing_permission_radio" type="radio" :button-variant="true" :button-variant-v-grouped="true">Default permission read+write</CheckboxRadioSwitch> <CheckboxRadioSwitch :checked.sync="sharingPermission" value="rw" name="sharing_permission_radio" type="radio" :button-variant="true" :button-variant-v-grouped="true">Default permission read+write</CheckboxRadioSwitch> </div> <div> Second toggle with two options <CheckboxRadioSwitch :checked.sync="sharingPermission" value="r" name="sharing_permission_radio" type="radio" :button-variant="true" :button-variant-v-grouped="true">Default permission read</CheckboxRadioSwitch> <CheckboxRadioSwitch :checked.sync="sharingPermission" value="rw" name="sharing_permission_radio" type="radio" :button-variant="true" :button-variant-v-grouped="true">Default permission read+write</CheckboxRadioSwitch> Other toggle not grouped <CheckboxRadioSwitch :checked.sync="sharingPermission" value="rw" name="sharing_permission_radio" type="radio" :button-variant="true">Default permission read+write</CheckboxRadioSwitch> <br> sharingPermission: {{ sharingPermission }} </div> </div> </template> <script> export default { data() { return { sharingPermission: 'r', } } } </script>

Which gives:

Notice that the corners of the second last checkbox are missing, since there is another one not belonging to the group afterwards. For the three grouped checkboxes wrapped in a container, it works fine.

So imho we only need a prop to indicate horizontal/vertical grouping (would be nice to figure that out by CSS as well, but I don't know how) and leave the rest to the proper markup (give a common container to the grouped boxes).

jancborchardt

Looks great design-wise now! :)

@eneiluj regarding the border-radius, it could also be done by adding a parent/container and using first- and last-child on that.

src/components/CheckboxRadioSwitch/CheckboxRadioSwitch.vue

julien-nc · 2022-06-20T12:41:00Z

So they need to be wrapped in a common container. Otherwise the markup will fail, without or with the grouped prop.

@raimund-schluessler What I meant is that from within the CheckboxRadioSwitch component, we can't know if all of the related CheckboxRadioSwitch are direct siblings (or if there are elements in between) so we need a prop for that. Of course they need to have a common direct parent but we need to know the intention of the developer as we can't guess it with CSS.

@jancborchardt If you mean adding one container around all the CheckboxRadioSwitch of a group, this is not possible as this component is only one element.
If you mean a container around each CheckboxRadioSwitch I don't see what difference it makes 😁.
Also, last-child is not enough as you can have other type of children in the same container so it would not select what we want for sure.

Thank you both for the feedback 💙 .

So we can now choose if we display them grouped with the buttonVariantGrouped prop. This has an effect on the first/last (selected with :first-of-type and :last-of-type) element's border radius and it merges the borders between elements.
When not grouped, all elements have rounded borders.

outdated

raimund-schluessler · 2022-06-20T12:48:16Z

So they need to be wrapped in a common container. Otherwise the markup will fail, without or with the grouped prop.

@raimund-schluessler What I meant is that from within the CheckboxRadioSwitch component, we can't know if all of the related CheckboxRadioSwitch are direct siblings (or if there are elements in between) so we need a prop for that. Of course they need to have a common direct parent but we need to know the intention of the developer as we can't guess it with CSS.

Yes, that's true. I just wanted to point out, that, if Checkboxes are grouped, there must not be any other Checkboxes in the container that don't belong to this group. Otherwise the :*-of-type selector won't lead to the desired result.

dartcafe · 2022-06-20T20:47:37Z

Why not adding a new component like https://github.com/nextcloud/polls/blob/master/src/js/components/Base/RadioGroupDiv.vue?

marcoambrosini

Hi, I think that the style of this component departs a bit from our design language, most notably due to the use of borders, which is something we've been trying to move away for some time. cc @jancborchardt
I know it might be too late for change but I'll leave an Idea of a similarly behaving component that we're using in our UI and whose style could probably fit here.

This also happens to be somewhat consistent with how they do radios in material design

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>

raimund-schluessler

The code looks good and works as intended.

Regarding the design, I think @marcoambrosini proposal looks nice as well, but that's up to @nextcloud/designers to decide and can be improved later on.

Also, I would be in favor of the grouped checkboxes container component @dartcafe proposed, because

grouped checkboxes obligatorily need a parent element containing only the checkboxes belonging to the group (otherwise the style fails) and a provided container component would make this more clear
then the container would only need a single prop indicating whether the boxes are grouped vertically or horizontally

So something like this could be the result:

<CheckboxRadioSwitchGroup direction="vertical/horizontal">
	<CheckboxRadioSwitch :checked.sync="sharingPermission" value="r" name="sharing_permission_radio" type="radio" :button-variant="true">Default permission read</CheckboxRadioSwitch>
	<CheckboxRadioSwitch :checked.sync="sharingPermission" value="rw" name="sharing_permission_radio" type="radio" :button-variant="true">Default permission read+write</CheckboxRadioSwitch>
	<CheckboxRadioSwitch :checked.sync="sharingPermission" value="rwx" name="sharing_permission_radio" type="radio" :button-variant="true">Default permission read+write+execute</CheckboxRadioSwitch>
</CheckboxRadioSwitchGroup>

julien-nc · 2022-06-22T13:19:05Z

@marcoambrosini I agree with @raimund-schluessler, in the end that's the decision of @nextcloud/designers. I completely understand if this is not merged because it does not go the same direction as most of the other components 😁 .

@raimund-schluessler @dartcafe About the CheckboxRadioSwitchGroup component, I think it would be nice indeed. The button-variant prop could be moved in CheckboxRadioSwitchGroup because all CheckboxRadioSwitch in a group will most likely have the same aspect.

dartcafe · 2022-06-22T13:22:38Z

I can start a PR and move our implementation to this repository. I guess there is some more work to do - besides the documentaion - because it currently fits into our use cases.

But I am also fine with it, if someone starts over.

marcoambrosini · 2022-06-22T15:39:23Z

I'm totally onboard with creating a new component for this!

julien-nc · 2022-06-23T09:13:36Z

@dartcafe I made a similar component like your RadioGroupDiv for the specific needs of a project: https://github.com/eneiluj/integration_visavid/blob/master/src/components/RadioElementSet.vue
But I guess passing the options as an array of objects makes prop validation harder and gives less control than the suggestion of @raimund-schluessler to insert the group content in the default slot.

@marcoambrosini So we have multiple ways to go. Which one makes more sense?

Leave the CheckboxRadioSwitch component untouched and create 2 new components: CheckboxRadioSwitchButton and CheckboxRadioSwitchButtonGroup. But CheckboxRadioSwitchButton would repeat most of CheckboxRadioSwitch's code...
Merge this PR and implement CheckboxRadioSwitchGroup
Any other idea?

skjnldsv · 2022-06-24T07:44:59Z

Leave the CheckboxRadioSwitch component untouched and create 2 new components: CheckboxRadioSwitchButton and CheckboxRadioSwitchButtonGroup. But CheckboxRadioSwitchButton would repeat most of CheckboxRadioSwitch's code...

Yeah, I'm not in favour.
This component ships fancy utils and methods about grouping, naming, events etc etc.
That would just duplicate the code.

I would be ok creating a CheckboxRadioSwitchGroup, that would be easy to check the parent from the CheckboxRadioSwitch component just to see which direction is the prop set.

Let's keep it simple people! 🚀

new CheckboxRadioSwitch prop to enable alternative style, for radio t…

860cc03

…ype only Signed-off-by: Julien Veyssier <eneiluj@posteo.net>

julien-nc requested review from jancborchardt, skjnldsv and marcoambrosini June 10, 2022 16:07

julien-nc added enhancement New feature or request 3. to review Waiting for reviews design Design, UX, interface and interaction design feature: checkbox-radio-switch Related to the checkbox-radio-switch component labels Jun 10, 2022

julien-nc added this to the 5.3.2 milestone Jun 10, 2022

This comment was marked as outdated.

Sign in to view

allow alternative style for all types

adef64b

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>

julien-nc force-pushed the enh/new-checkboxradioswitch-radio-style branch from 73b9690 to adef64b Compare June 12, 2022 16:14

This comment was marked as outdated.

Sign in to view

jancborchardt requested changes Jun 16, 2022

View reviewed changes

julien-nc force-pushed the enh/new-checkboxradioswitch-radio-style branch from 15e41b7 to f3331d6 Compare June 17, 2022 12:25

skjnldsv previously requested changes Jun 17, 2022

View reviewed changes

use :first-of-type and :last-of-type for grouped elements

6732a91

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>

julien-nc force-pushed the enh/new-checkboxradioswitch-radio-style branch from f3331d6 to 16d87a8 Compare June 20, 2022 10:00

julien-nc requested review from jancborchardt and skjnldsv June 20, 2022 10:11

raimund-schluessler requested changes Jun 20, 2022

View reviewed changes

jancborchardt approved these changes Jun 20, 2022

View reviewed changes

julien-nc force-pushed the enh/new-checkboxradioswitch-radio-style branch 2 times, most recently from dfb4afc to 5b7f490 Compare June 20, 2022 12:26

raimund-schluessler reviewed Jun 20, 2022

View reviewed changes

src/components/CheckboxRadioSwitch/CheckboxRadioSwitch.vue Outdated Show resolved Hide resolved

raimund-schluessler reviewed Jun 20, 2022

View reviewed changes

src/components/CheckboxRadioSwitch/CheckboxRadioSwitch.vue Outdated Show resolved Hide resolved

julien-nc requested a review from raimund-schluessler June 20, 2022 12:41

marcoambrosini reviewed Jun 21, 2022

View reviewed changes

add buttonVariantGrouped prop to choose group style

114301e

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>

julien-nc force-pushed the enh/new-checkboxradioswitch-radio-style branch from 5b7f490 to 114301e Compare June 21, 2022 22:21

raimund-schluessler approved these changes Jun 22, 2022

View reviewed changes

julien-nc merged commit 6e033de into master Jun 29, 2022

julien-nc deleted the enh/new-checkboxradioswitch-radio-style branch June 29, 2022 13:31

juliushaertl modified the milestones: 5.3.2, 6.0.0 Aug 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alternative style for CheckboxRadioSwitch (radio type) #2757

Alternative style for CheckboxRadioSwitch (radio type) #2757

julien-nc commented Jun 10, 2022 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

jancborchardt left a comment •

edited by julien-nc

Loading

julien-nc commented Jun 17, 2022

skjnldsv Jun 17, 2022

julien-nc Jun 20, 2022 •

edited

Loading

raimund-schluessler Jun 20, 2022

raimund-schluessler Jun 20, 2022

jancborchardt left a comment

julien-nc commented Jun 20, 2022

raimund-schluessler commented Jun 20, 2022

dartcafe commented Jun 20, 2022

marcoambrosini left a comment

raimund-schluessler left a comment •

edited

Loading

julien-nc commented Jun 22, 2022

dartcafe commented Jun 22, 2022 •

edited

Loading

marcoambrosini commented Jun 22, 2022

julien-nc commented Jun 23, 2022

skjnldsv commented Jun 24, 2022

Alternative style for CheckboxRadioSwitch (radio type) #2757

Alternative style for CheckboxRadioSwitch (radio type) #2757

Conversation

julien-nc commented Jun 10, 2022 • edited Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

jancborchardt left a comment • edited by julien-nc Loading

Choose a reason for hiding this comment

julien-nc commented Jun 17, 2022

skjnldsv Jun 17, 2022

Choose a reason for hiding this comment

julien-nc Jun 20, 2022 • edited Loading

Choose a reason for hiding this comment

raimund-schluessler Jun 20, 2022

Choose a reason for hiding this comment

raimund-schluessler Jun 20, 2022

Choose a reason for hiding this comment

jancborchardt left a comment

Choose a reason for hiding this comment

julien-nc commented Jun 20, 2022

raimund-schluessler commented Jun 20, 2022

dartcafe commented Jun 20, 2022

marcoambrosini left a comment

Choose a reason for hiding this comment

raimund-schluessler left a comment • edited Loading

Choose a reason for hiding this comment

julien-nc commented Jun 22, 2022

dartcafe commented Jun 22, 2022 • edited Loading

marcoambrosini commented Jun 22, 2022

julien-nc commented Jun 23, 2022

skjnldsv commented Jun 24, 2022

julien-nc commented Jun 10, 2022 •

edited

Loading

jancborchardt left a comment •

edited by julien-nc

Loading

julien-nc Jun 20, 2022 •

edited

Loading

raimund-schluessler left a comment •

edited

Loading

dartcafe commented Jun 22, 2022 •

edited

Loading